Skip to content

GH-49415: [C++] Don't change map type key/item/value field names#49416

Merged
kou merged 1 commit intoapache:mainfrom
kou:cpp-map-name
Mar 2, 2026
Merged

GH-49415: [C++] Don't change map type key/item/value field names#49416
kou merged 1 commit intoapache:mainfrom
kou:cpp-map-name

Conversation

@kou
Copy link
Member

@kou kou commented Mar 1, 2026

Rationale for this change

The current implementation forces using key/value/entries as key/item/value field names and ignores field names in FlatBuffers.

What changes are included in this PR?

Use field names in FlatBuffers.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

This PR includes breaking changes to public APIs. If an application depends on key/value/entriesas map type's field names, an application may not work when incoming Apache Arrow data doesn't usekey/value/entries` as map type's field names.

@kou kou requested review from lidavidm, wgtmac and wjones127 March 1, 2026 06:11
@github-actions
Copy link

github-actions bot commented Mar 1, 2026

⚠️ GitHub issue #49415 has been automatically assigned in GitHub to PR creator.

@kou
Copy link
Member Author

kou commented Mar 1, 2026

@wgtmac @lidavidm @wjones127 What do you think about this? This code was changed by #35298 and you worked on the PR.

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if any users may have inadvertently come to depend on this behavior. Should we highlight it as a potentially breaking change?

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Mar 1, 2026
@wgtmac
Copy link
Member

wgtmac commented Mar 1, 2026

I think I've recently encountered a similar issue where Parquet and Arrow uses different hardcoded field names for map type: #49218 (comment)

@kou
Copy link
Member Author

kou commented Mar 1, 2026

I wonder if any users may have inadvertently come to depend on this behavior. Should we highlight it as a potentially breaking change?

OK. I've updated the PR description for it.

FYI: @raulcd you may be a release manager for the next major release. We want to highlight this change in the release announce blog post.

@raulcd raulcd added the Breaking Change Includes a breaking change to the API label Mar 2, 2026
@kou
Copy link
Member Author

kou commented Mar 2, 2026

I'll merge this.

@kou kou merged commit 0bddf5d into apache:main Mar 2, 2026
72 of 76 checks passed
@kou kou deleted the cpp-map-name branch March 2, 2026 23:58
@kou kou removed the awaiting merge Awaiting merge label Mar 2, 2026
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 0bddf5d.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them.

@kou
Copy link
Member Author

kou commented Mar 3, 2026

Hmm. The Go implementation also ignores key/item/value field names...: https://github.com/apache/arrow-go/blob/2de8b123a24f0e687fbfc590a0019c803037a076/arrow/ipc/metadata.go#L846

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking Change Includes a breaking change to the API Component: C++

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants